Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support proxy environment for docker client #2424

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Support proxy environment for docker client #2424

merged 1 commit into from
Aug 27, 2024
+2 −0

Conversation

PMExtra
Copy link
Contributor

@PMExtra PMExtra commented May 17, 2024 •
edited
Loading

Proxy environment works fine with docker-cli:

> export http_proxy=http://my_proxy:8080
> export DOCKER_HOST=tcp://my_docker_host_through_proxy
> docker info
# Works fine
> docker image inspect foo:bar
# Works fine

But does not work with skopeo:

> export http_proxy=http://my_proxy:8080
> skopeo inspect --daemon-host http://my_docker_host_through_proxy docker-daemon:foo:bar
FATA[0000] Error parsing image name "docker-daemon:foo:bar": initializing docker engine client: unable to parse docker host `my_docker_host_through_proxy`

It's just an example, I need to copy image to the docker-daemon host in actual.

After this pull request was patched, skopeo also worked fine with the proxy environment.

Note that: I used a plain HTTP connection with docker-daemon, so it should be http:// in skepeo but tcp:// in docker-cli.

mtrmac
mtrmac requested changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

After this pull request was patched, skopeo also worked fine with the proxy environment.

This does not even compile, so I can’t see how this could have possibly worked.

Also, per https://github.com/containers/image/blob/main/CONTRIBUTING.md#sign-your-prs :

Use your real name (sorry, no pseudonyms or anonymous contributions.)

@PMExtra PMExtra requested a review from mtrmac May 18, 2024 08:54
@PMExtra
Copy link
Contributor Author

PMExtra commented May 21, 2024

@mtrmac I've fixed it. Please let me know if there is anything else I need to do.

@rhatdan
Copy link
Member

rhatdan commented May 21, 2024

LGTM

mtrmac
mtrmac reviewed May 21, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this makes sense doing, then the http: and https: protocols should support proxies equally.

(Alternatively, AFAICS we don’t actually need to configure this manually: comparing https://github.com/docker/cli/blob/57a1180c52ad7633dcf2fe1515ec2601ffb01944/cli/context/docker/load.go#L85 and looking at the implementation, if WithHttpClient is included before WithHost, that triggers sockets.ConfigureTransport and that sets this, along, with a few other options. But that’s undocumented and very nonobvious; it’s really only a confirmation that ProxyFromEnvironment can be appropriate here.)

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 27, 2024
@rhatdan
Copy link
Member

rhatdan commented Aug 7, 2024

@PMExtra still working on this?

@PMExtra
Copy link
Contributor Author

PMExtra commented Aug 7, 2024

I don't know what I should do. I think it's ready for merging.

@rhatdan
Copy link
Member

rhatdan commented Aug 7, 2024

@mtrmac what would you like to see done?

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 7, 2024

If this makes sense doing, then the http: and https: protocols should support proxies equally.

This only adds them to the former; add support for the latter.

@PMExtra
Copy link
Contributor Author

PMExtra commented Aug 21, 2024 •
edited
Loading

@mtrmac I'm not sure what you mean. http.ProxyFromEnvironment can support several variables such as HTTP_PROXY, HTTPS_PROXY, and NO_PROXY.

If you set HTTPS_PROXY, it will work with https:.

@PMExtra
Copy link
Contributor Author

PMExtra commented Aug 21, 2024

@mtrmac see https://pkg.go.dev/net/http#ProxyFromEnvironment

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 21, 2024

The httpConfig function is not used for https, so whether http.ProxyFromEnvironment would have worked for https is irrelevant when this PR is not invoking it in that case.

@PMExtra

This comment was marked as off-topic.

Sign in to view
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 22, 2024

That example does not reference the httpConfig function (and, more to the point, its caller) at all, and is irrelevant to my objection.

@PMExtra
Copy link
Contributor Author

PMExtra commented Aug 23, 2024

@mtrmac Now I finally understand what you meant, and I apologize for the previous misunderstanding.

@PMExtra PMExtra requested a review from mtrmac August 27, 2024 09:12
mtrmac
mtrmac reviewed Aug 27, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Code LGTM, please squash into one commit and rebase.

@PMExtra PMExtra requested a review from mtrmac August 27, 2024 13:54
mtrmac
mtrmac approved these changes Aug 27, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mtrmac mtrmac merged commit d70ed8f into containers:main Aug 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac approved these changes

Assignees
No one assigned
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PMExtra @rhatdan @mtrmac